Add Cross-program invocations #9582

jackcmay · 2020-04-19T05:42:38Z

Problem

Programs cannot invoke other programs
Programs cannot generate their own signatures in process_instruction as defined in the Cross-Program Invocations design.

Summary of Changes

Implements the following proposals

This PR has been whittled down to the essential changes by breaking up all dependent and related changes into other PRs that have already been committed.

Fixes #

jackcmay · 2020-04-20T18:47:31Z

Hitting an error loading the solana_bpf_loader_program on linux:
[2020-04-20T17:19:33.608471890Z WARN solana_runtime::native_loader] Failed to load: Custom { kind: Other, error: "/solana/target/debug/deps/libsolana_bpf_loader_program.so: cannot allocate memory in static TLS block" }

Debugging... wip label for now

codecov · 2020-04-21T01:50:42Z

Codecov Report

Merging #9582 into master will decrease coverage by 0.2%.
The diff coverage is 61.2%.

@@           Coverage Diff            @@
##           master   #9582     +/-   ##
========================================
- Coverage    80.5%   80.3%   -0.3%     
========================================
  Files         280     281      +1     
  Lines       64031   64615    +584     
========================================
+ Hits        51592   51897    +305     
- Misses      12439   12718    +279

jackcmay · 2020-04-21T17:43:54Z

Fixed and ready for review, there's a lot in there so let the critiques commence!

mvines

Really nice work!

programs/bpf_loader/src/helpers.rs

garious

Looks great to me. I found the term "cross-program" to be a little confusing. I think it's used to mean at least "foreign function call" and "cross-program process_instruction". Even though I've been calling the function process_instruction(), it looks like calling it invoke() would nicely disambiguate it from MessageProcessor::process_instruction() and allow you to use "invoke" or "invocation" in places you're currently using "cross_program". Maybe even "solana_ffi::invoke".

As for the new runtime rules, I didn't review it in detail, but it all looked consistent with the design and what we've discussed on Discord. And lots of tests, so not a lot to worry about. Hoping we can get this merged soon and into v1.2.0.

jackcmay · 2020-04-22T21:27:06Z

@garious I'm a fan of process_instruction() since it both conveys what is happening (processing the provided instruction) and is consistent with the rest of the runtime. The term "cross-program" I could take or leave. I like "solana_ffi" though it seems to imply a lot more then we need or want to. Calling out to other programs is natural and in our case there isn't anything to be gained by letting the caller know that it might be calling another language. Maybe invocation::process_instruction()?

garious · 2020-04-22T21:44:51Z

@jackcmay, how about solana_invoke::process_instruction?

garious · 2020-04-22T21:46:31Z

solana_call?

jackcmay · 2020-04-22T21:52:35Z

@greg The full path would be comprised of solana_sdk:: followed by the file name followed by the name of the function. So for example:

solana_sdk::invoke::process_instruction
solana_sdk::invocation::process_instruction

Did you have something different in mind?

garious · 2020-04-22T21:55:54Z

@jackcmay Got it. I missed that. What about solana_sdk::program::process_instruction()?

jackcmay · 2020-04-22T22:03:22Z

Originally had it in solana_sdk::program_utils::process_instruction() but that would require featurizing those functions rather than featurize the whole file like in solana_sdk::cross_program. I try to avoid speckled featurization is possible. program seems too generic and using process_instruction does conflict with a program's entrypoint function which we usually call process_instruction.

Along your lines of thinking we could do: solana_sdk::program::invoke

garious · 2020-04-22T22:06:52Z

solana_sdk::program::invoke

I like that a lot

programs/bpf_loader/src/helpers.rs

programs/bpf/rust/invoked/src/instruction.rs

jstarry · 2020-04-23T03:08:56Z

programs/bpf_loader/src/helpers.rs

+
+    // Process instruction
+
+    let program_account = (*accounts[program_id_index]).clone();


I think we only need to clone the program data if the program is also an instruction account.

What are you referring to as an instruction account?

I meant an account that is passed as an argument to an instruction

programs/bpf_loader/src/helpers.rs

jstarry · 2020-04-23T10:28:25Z

programs/bpf/rust/invoke/src/lib.rs

+const DERIVED_KEY2_INDEX: usize = 7;
+
+entrypoint!(process_instruction);
+fn process_instruction(


I think there should be a test for cross program instructions with only a subset of the caller accounts.

Also, is there a min / max number of accounts that can be passed to cross_program::process_instruction?

Good idea will add those tests. Also, there is currently no limit to the number of accounts that can be passed beside the memory restrictions of the calling process. This could be a ddos vulnerability

On second thought, is there a reason to do this? The runtime only passes the accounts listed in the instruction to the invoked program. Breaking up the accounts vector isn't a fun thing to do

Ok maybe I'm missing something. What if a program is called with 2 account arguments and wants to call a program that needs 3?

Then it fails, programs need to be passed all the accounts it needs to pass to any invokes calls it intends to make

Ah I think I figured out the disconnect!

I was imagining that the caller program could rearrange, filter, dupe accounts etc. and then pass that new account slice to the called function. But that's overkill because as you mentioned, the cross program instruction just needs to reference the indices of the accounts from the original account slice.

As it stands, the caller could still choose to pass only a subset of the accounts to the called program I think. I don't see a check to make sure that all accounts in pre_accounts are present in the accounts slice

Is there a reason that the caller needs to pass the accounts slice back out through sol_process_signed_instruction_? Can't we check for them how accounts have been modified?

~~Ohhh we are doing that. Sorry!!!~~

Oh, no we aren't actually 😅

Does this make sense? Do we need the caller to pass through all the account infos? Don't we already know where the accounts are memory mapped?

This issue is summarized in #9711 and is planned as a followup change to the PR

sdk/src/cross_program.rs

runtime/src/message_processor.rs

jstarry

looks like malicious programs could read the entirety of a memory region if they mess with the serialized lengths. Is that problematic?

jackcmay · 2020-04-23T19:08:37Z

@jstarry We can't enforce that the passed buffers are sized correctly or even pointing to anything valid. We can only verify that the pointer and size of the buffer have been granted to the calling program. The called program only has access to what has been passed by the caller, aka the called can't read or write memory of the caller's address space outside of what the caller passed.

jstarry · 2020-04-24T01:59:58Z

Ok, sounds good!

Pull request has been modified.

jackcmay · 2020-04-28T16:38:30Z

solana_sdk::program::invoke

I like that a lot

@garious Take a look at the new naming, like how it looks now that its in?

jackcmay · 2020-04-28T16:51:48Z

@mvines @jstarry @garious
I disabled reentrancy in this PR, we can open it up again when we work out those policy details. Here is what the reentrancy check looks like, self.program_ids is the current call stack of program ids

        if self.program_ids.contains(key) && self.program_ids.last() != Some(key) {
            // Reentrancy not allowed unless caller is calling itself
            return Err(InstructionError::ReentrancyNotAllowed);
        }

Here are also some follow-up issues that came out of the review or todos:

Anything else?

garious · 2020-04-28T16:58:07Z

@garious Take a look at the new naming, like how it looks now that its in?

Love it!

ryoqun · 2020-05-05T05:39:50Z

runtime/src/message_processor.rs

+            // Verify the called program has not misbehaved
+            result = invoke_context.verify_and_update(message, instruction, signers, accounts);
+        }
+        invoke_context.pop();


@jackcmay Sorry for being after-party comment, but I just started to looking the program-related code. :)

This invoke_context.pop() isn't always be paired with invoke_context.push(...) when keyed_accounts[0].owner()? fails to borrow. Is that ok?

And qq: is cross-program invocation can be gracefully handled inside a invoker program in some way when invokee's result is InstructionError?
Then, this verification might start to reference a wrong program_id.

I think to it's better to avoid these kind of things like with closure surrounded by push() and pop()

Yeah, that will never fail in this implementation so I left it. I recently updated this code to extend cross-program invocation to native programs and in those changes the case you mention no longer exists.

I'm not quite following the "qq", if the invoke fails the result is held and context poped before returning. Can you elaborate?

jackcmay force-pushed the cpi branch from 3b332f4 to c7e1097 Compare April 20, 2020 05:17

jackcmay added the work in progress This isn't quite right yet label Apr 20, 2020

jackcmay force-pushed the cpi branch from 965417a to c3008af Compare April 21, 2020 00:00

jackcmay removed the work in progress This isn't quite right yet label Apr 21, 2020

jackcmay force-pushed the cpi branch from c3008af to 9620d86 Compare April 21, 2020 17:43

mvines reviewed Apr 22, 2020

View reviewed changes

programs/bpf_loader/src/helpers.rs Outdated Show resolved Hide resolved

programs/bpf_loader/src/helpers.rs Outdated Show resolved Hide resolved

garious previously approved these changes Apr 22, 2020

View reviewed changes

jstarry reviewed Apr 23, 2020

View reviewed changes

jackcmay added 3 commits April 28, 2020 00:36

Add Cross-program invocations

8ef62b8

feedback

34f899a

Disable reentrancy

ddd126a

jackcmay force-pushed the cpi branch from cee6c3a to ddd126a Compare April 28, 2020 07:36

jackcmay merged commit 068f12f into solana-labs:master Apr 28, 2020

jackcmay deleted the cpi branch April 28, 2020 21:34

ryoqun reviewed May 5, 2020

View reviewed changes

ryoqun mentioned this pull request May 8, 2020

Maintain sysvar balances for consistent market cap. #9936

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Cross-program invocations #9582

Add Cross-program invocations #9582

jackcmay commented Apr 19, 2020

jackcmay commented Apr 20, 2020

codecov bot commented Apr 21, 2020 •

edited

Loading

jackcmay commented Apr 21, 2020 •

edited

Loading

mvines left a comment

garious left a comment

jackcmay commented Apr 22, 2020

garious commented Apr 22, 2020

garious commented Apr 22, 2020

jackcmay commented Apr 22, 2020

garious commented Apr 22, 2020

jackcmay commented Apr 22, 2020

garious commented Apr 22, 2020 •

edited

Loading

jstarry Apr 23, 2020

jackcmay Apr 23, 2020

jstarry Apr 24, 2020

jstarry Apr 23, 2020

jackcmay Apr 23, 2020

jackcmay Apr 24, 2020

jstarry Apr 24, 2020

jackcmay Apr 24, 2020

jstarry Apr 24, 2020

jstarry Apr 24, 2020

jstarry Apr 24, 2020 •

edited

Loading

jstarry Apr 24, 2020

jackcmay Apr 24, 2020

jstarry left a comment

jackcmay commented Apr 23, 2020

jstarry commented Apr 24, 2020

jackcmay commented Apr 28, 2020

jackcmay commented Apr 28, 2020 •

edited

Loading

garious commented Apr 28, 2020

ryoqun May 5, 2020

jackcmay May 22, 2020


		// Process instruction

		let program_account = (*accounts[program_id_index]).clone();

Add Cross-program invocations #9582

Add Cross-program invocations #9582

Conversation

jackcmay commented Apr 19, 2020

Problem

Summary of Changes

jackcmay commented Apr 20, 2020

codecov bot commented Apr 21, 2020 • edited Loading

Codecov Report

jackcmay commented Apr 21, 2020 • edited Loading

mvines left a comment

Choose a reason for hiding this comment

garious left a comment

Choose a reason for hiding this comment

jackcmay commented Apr 22, 2020

garious commented Apr 22, 2020

garious commented Apr 22, 2020

jackcmay commented Apr 22, 2020

garious commented Apr 22, 2020

jackcmay commented Apr 22, 2020

garious commented Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarry Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarry left a comment

Choose a reason for hiding this comment

jackcmay commented Apr 23, 2020

jstarry commented Apr 24, 2020

jackcmay commented Apr 28, 2020

jackcmay commented Apr 28, 2020 • edited Loading

garious commented Apr 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 21, 2020 •

edited

Loading

jackcmay commented Apr 21, 2020 •

edited

Loading

garious commented Apr 22, 2020 •

edited

Loading

jstarry Apr 24, 2020 •

edited

Loading

jackcmay commented Apr 28, 2020 •

edited

Loading